feat: introduce RoleGroupModel #1124
Conversation
Reviewer's GuideThis PR introduces a new RoleGroupModel proxy to group rows by a deduplication role, integrates it into the taskmanager build setup, and adds unit tests to validate its grouping logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @BLumia - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
-
columnCount() should check for a valid sourceModel pointer. (link)
-
roleNames() should check for a valid sourceModel pointer. (link)
-
Implement a destructor (or cleanup routine) to delete the QList* in m_rowMap to avoid leaking those dynamically allocated lists.
-
Emit the deduplicationRoleChanged signal in setDeduplicationRole() after changing m_roleForDeduplication to keep QML bindings and observers in sync.
-
Either remove the declared hasIndex() override in the header or provide a matching implementation to prevent linker errors.
Here's what I looked at during the review
- 🔴 General issues: 2 blocking issues, 7 other issues
- 🟡 Testing: 2 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| if (sourceModel()) { | ||
| sourceModel()->disconnect(this); |
There was a problem hiding this comment.
issue (bug_risk): Disconnecting all signals from the previous source model may unintentionally break other connections.
disconnect(this) removes all sourceModel→this connections, risking other slots. Instead, disconnect only the signals you connected or store QMetaObject::Connection objects to manage them.
| return m_rowMap.size(); | ||
| } | ||
|
|
||
| int RoleGroupModel::columnCount(const QModelIndex &parent) const |
There was a problem hiding this comment.
issue (bug_risk): columnCount() should check for a valid sourceModel pointer.
Add a null check for sourceModel() and return 0 if it’s unset to prevent a crash.
| return sourceModel()->columnCount(parent); | ||
| } | ||
|
|
||
| QHash<int, QByteArray> RoleGroupModel::roleNames() const |
There was a problem hiding this comment.
issue (bug_risk): roleNames() should check for a valid sourceModel pointer.
Add a null check and return an empty QHash if sourceModel() is null.
| return sourceModel()->roleNames(); | ||
| } | ||
|
|
||
| QVariant RoleGroupModel::data(const QModelIndex &index, int role) const |
There was a problem hiding this comment.
issue: data() should validate the index and sourceModel pointer.
Return a default QVariant when the index is invalid or sourceModel() is null to prevent crashes.
| return createIndex(pos, 0); | ||
| } | ||
|
|
||
| QModelIndex RoleGroupModel::mapToSource(const QModelIndex &proxyIndex) const |
There was a problem hiding this comment.
issue: mapToSource() should validate proxyIndex and sourceModel pointer.
Return a default QModelIndex when proxyIndex is invalid or sourceModel() is null to prevent crashes.
| return sourceModel()->index(list->value(0), 0); | ||
| } | ||
|
|
||
| QModelIndex RoleGroupModel::mapFromSource(const QModelIndex &sourceIndex) const |
There was a problem hiding this comment.
issue (bug_risk): mapFromSource() should validate sourceIndex and sourceModel pointer.
Return a default QModelIndex if sourceIndex is invalid or sourceModel() is null to avoid crashes.
|
|
||
| #include "rolegroupmodel.h" | ||
|
|
||
| TEST(RoleGroupModel, RowCountTest) |
There was a problem hiding this comment.
suggestion (testing): Consider renaming test or splitting into multiple more focused tests.
Use a more descriptive name (e.g., TestGroupingAndDataRetrieval) or split this into focused tests—one for grouping, one for data access, and one for row removal—to improve readability and failure diagnosis.
| QSignalSpy spy(&model, &QAbstractItemModel::rowsRemoved); | ||
| QSignalSpy spys(&model, &QAbstractItemModel::rowsInserted); |
There was a problem hiding this comment.
issue (testing): Unused signal spies should be removed or utilized.
Either remove the unused QSignalSpy instances or add assertions (e.g., EXPECT_EQ(spy.count(), expected_count)) to verify the signals.
BLumia
force-pushed
the
rolegroupmodel
branch
2 times, most recently
from
31d983d to
de1c0b9
Compare
deepin pr auto review代码审查意见：
以上是代码审查意见，需要根据实际情况进行调整和修改。 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
3 participants
ported from the original feature development branch for combined icons
Summary by Sourcery
Introduce RoleGroupModel to group items in the taskmanager panel by a specified data role and integrate it into the build system.
New Features:
Build:
Tests: